CUDA: enable the CUB path on HIP via hipCUB - #26592
Conversation
The CUB-based ARGSORT/TOP_K (and sum/mean/cumsum) paths were compiled out for HIP, so those ops fell back to the shared-memory bitonic sort. That sort uses one thread per element, so it is limited to rows of 1024 elements; anything wider was reported as unsupported and ran on the CPU. For deepseek4 that meant every lightning-indexer TOP_K above 1024 context went to the host, adding a graph split per layer. hipCUB provides the device-wide sorts these paths need - the rocPRIM backend has DeviceRadixSort, DeviceSegmentedRadixSort, DeviceSegmentedSort, DeviceReduce and DeviceScan - so include it through a small cub-compat shim that aliases the cub namespace, and define GGML_CUDA_USE_CUB for HIP as well. hipCUB has no DeviceTopK and does not define CCCL_*, so CUB_TOP_K_AVAILABLE and STRIDED_ITERATOR_AVAILABLE stay disabled there: TOP_K keeps using the argsort + copy path and argsort keeps using the init_offsets kernel instead of a strided iterator. vendors/hip.h gains the stream-capture defines that the CUB argsort path needs now that it is compiled for HIP.
|
@ggml-org/ci Looks like CI hip workflows need to be updated to install hipcub dev package for this. Not sure about HIP Windows release, but I guess that's something to worry about later. |
|
So test this PR here with 4x gfx1200 9060xt and 1x gfx1151. No crash at 68727 PP context. |
|
@fairydreaming isn't hipcub included in rocm? |
@Geramy No idea, my adventure with AMD GPUs ended during Radeon R9 290 times. By the way, could you run some Thanks! |
|
@fairydreaming from AMD GitHub page directly |
|
Cline is working now including reasoning and cache with up to 172k tokens for me over rpc. Edit: by 193k tokens ca. 600mb VRAM increase per GPU i can observe. |
Well for some reason hipCUB header files were missing in the CI container during compilation, I see that it installs Ubuntu rocblas-dev and hipblas-dev packages, so maybe they also need to install libhipcub-dev or something. |
Yeah perhaps it depends on the ROCm version we are on too. Who will or can take care of that? |
@Geramy I don't know, if no one fixes it I'll see what I can do tomorrow (going to sleep soon). |
|
Confirmed to work on a Strix Halo: with this patch the tg speed stays correct even after >4-5k context (instead of having a big drop to 5 tok/s). 🎉 However, I'm wondering if the prefill speed doesn't get worse faster than with #26493, but that can be totally wrong, don't read too much into this. |
|
HIP quality check CI found some missing CUB function call return value error checks, so I added them. Compilation worked fine with added |
Co-authored-by: Georgi Gerganov <ggerganov@gmail.com>
|
@fairydreaming thanks! I didn’t see any of the CIs run or get approved to run did you do a few manually in actions? |
|
@Geramy Yeah, in this PR I have to approve workflows manually to run and they reset after each new commit. Probably you can see them in Actions history (ran about 4 hours ago). Now let's wait for someone from ggml-cuda to review and approve this. |
Are you able to publish the numbers you get? On ROCm-7.14 I am not getting better performance. |
14.2 tok/s generation (this PR) vs 14 tok/s generation (bitonic PR) - this is not a rigorous benchmark like yours though, this is just a casual observation for a test prompt. Even a rigorous benchmark would not be directly comparable because I am running over RPC and CUDA (Host) + RPC (ROCm) crashes on master without a PR to provide ROCm-based topk. Edit: Did you also try with TheRock-Nightly? |
The original issue is DeepSeek 4 Flash falls back to cpu top-k causing huge performance drop. You have to benchmark above 4096 to see the drop on main but this drop was over multiple GPUs. |
Oh, you mean generating more than 4K, not depth? |
…pecific files for cub, dropped redundent includes and added a hip version check for hip cub inclusion, moved around a few includes, and added a test @fairydreaming originally had me run.
Right the original issue is when I believe token input goes over like 50xx it causes top-k to fall back to the CPU top-k / argsort function. There is a optimization to be had later one perhaps, I do notice we "define" top-k and argsort as the same backend function when top-k doesn't have to be sorted, I would think this would save some compute and a little bit of memory bandwidth but that's a discussion for another PR, and this is all cuda / hip related backend, not vulkan. |
what do you get on 7.13 or 7.2.3 or are we at .4 now? just wondering if its a 7.14 issue or a hipcub issue, I can look into hipcub a little more and see if there is something I can do about the performance. |
|
Regarding the questionable performance improvements observed by some people I suppose it depends on your specific CPU/GPU combination and interface between them. For example on my machine (Epyc 9374F + RTX PRO 6000 Max-Q) I have: CPUGPU (CUB argsort)So it's not like GPU is always faster. My guess is that performance improvements will be mostly visible on platforms with slow CPU, slow CPU memory or slow CPU-GPU interconnect. |
…checkpoint-all-opt Merge ROCm-specific optimizations: - PR ggml-org#26592: enable CUB path on HIP via hipCUB (top_k support) - ci : install hipCUB headers - perf: sweep quants for best performance - fix: q2_k mma - fix: q6_k mma - Placed MMA gateway over WMMA gateway - ggml : tune RDNA4 MMVQ warps for K-quants - server: persist checkpoints via sidecar file for hybrid/recurrent slot save/restore - heartbeat: restore CUDA noop kernel, add Vulkan back in, rename --no-sleep to --gpu-heartbeat
|
@fairydreaming I would 100% agree with you on this. cub is generally more expensive, now we could create a top-k function if llama.cpp wants but right now its the same function as argsort, top-k specifies it doesn't need a sort function, that might help, might. but this PR is more about fixing a limitation then performance I have the other PR I closed that one for this one, the other one was a extension and optimization, we can say anything more than X top-k gets moved to cub otherwise run with the new element per thread packing top-k function, Just a thought anyway it sounds like more work and more "gotchas" for maintenance. |
|
After the recent changes, all is working correctly for me now! (i.e. compiling works fine by default and the pragma stuff above is not necessary). @Geramy I agree with your views on the purpose of the PR, as the op (for >1024) is currently missing from ROCm, which in some cases (eg ROCm over RPC) causes a crash, so adding it to the ROCm backend is important. It may be possible to improve on-device-topk performance by using a dedicated primitive like rocprim::topk, but I completely agree that should be some later PR to tune the performance. |
|
@matt23654 Do I understand correctly that |
|
@fairydreaming It is my understanding that I believe your expert knowledge that for dsv4 a batched topk would be even better. ROCm has recently added ROCm/rocm-libraries#3646 -- At the time of this PR, only ROCm unstable had implemented a faster algorithm, so only very new ROCm can possibly be faster. |
|
@matt23654 I believe rocm 7.14 will be landing soon as well. @superm1 has a PR to bump to 7.14 |
|
@fairydreaming How soon do you think we can merge this? I would like to get started on the next PR that jumps at me haha. |
Yes but this solution is slow during prompt processing due to calling it iteratively for each row in a loop, hopefully replacing it with
Lightning indexer scores are contiguous, so it seems to be exactly what we need. |
@Geramy Hard to say, I'm not in charge here. People review things when they have free time, which is kind of random. But if you are bored already how about getting lightning indexer WMMA kernel working on ROCm? Should get AMD users a nice performance bump. You said something earlier about rocWMMA being deprecated, but isn't that only about rocWMMA repo being deprecated (moved to rocm-libraries) and not the API? |
I was under the impression from some commits and comments that rocWMMA was removed from llama.cpp and replaced with MMA I’ll have to go back and double check that. Yes I totally could work on lightning indexer, maybe if the above is true I just bring it to MMA instead. |
@Geramy Flash attention WMMA kernel was removed recently because there is MMA one that is faster. But it's not the case with lightning indexer kernel - still uses WMMA (but only for CUDA at the moment, ROCm falls back to slower vector kernel). |
I believe that is the case, I recall @JohannesGaessler working on this 6-7 months ago, in fact I don't use rocmWMMA builds any longer and recall benchmarking them against the new kernels and the new kernels performing on par or better. |
|
yes rocwamma support in fattn_ext is gone. |
…ind to test ds4-fast top-k functionality.
| #if !defined(GGML_USE_HIP) && !defined(GGML_USE_MUSA) && CUDART_VERSION >= 11070 | ||
| #define USE_CUB | ||
| #endif // !defined(GGML_USE_HIP) && !defined(GGML_USE_MUSA) && CUDART_VERSION >= 11070 | ||
|
|
||
| #ifdef USE_CUB |
There was a problem hiding this comment.
| #if !defined(GGML_USE_HIP) && !defined(GGML_USE_MUSA) && CUDART_VERSION >= 11070 | |
| #define USE_CUB | |
| #endif // !defined(GGML_USE_HIP) && !defined(GGML_USE_MUSA) && CUDART_VERSION >= 11070 | |
| #ifdef USE_CUB | |
| #ifdef GGML_CUDA_USE_CUB |
| #ifdef USE_CUB | ||
| #include <cub/cub.cuh> | ||
| using namespace cub; | ||
| #endif // USE_CUB |
There was a problem hiding this comment.
| #endif // USE_CUB | |
| #endif // GGML_CUDA_USE_CUB |
There was a problem hiding this comment.
..and update the other USE_CUB in this file...
Overview
Since the CUB-based ARGSORT/TOP_K paths were never running on a HIP based device, I have enabled them and added a few missing hip graph functions. Originally those ops fell back to the shared-memory bitonic sort. That sort uses one thread per element, so it is limited to rows of 1024 elements; anything wider was reported as unsupported and ran on the CPU.
Additional information
I have added stream-capture functions missing from vendors/hip.h which is required in CUB argsort.
DeepSeek-V4-Flash UD-IQ1_S — 284B MoE, 78 GB
pp512prompt processing
tg32token generation
Qwen3.6-35B-A3B UD-Q4_K_XL
pp512prompt processing
tg32token generation
Requirements
@IMbackK @am17an @pwilkin